test: Add datafusion.format.* configs test coverage#21355
Conversation
alamb
left a comment
alamb
left a comment
There was a problem hiding this comment.
Thanks @erenavsarogullari -- I think these are nice improvements in coverage. It would be even nicer to get some functional coverage too if they don't yet exist
| ## Test datafusion.format.* configurations ## | ||
| ############################################# | ||
| query T | ||
| SELECT name FROM information_schema.df_settings WHERE name LIKE 'datafusion.format.%' ORDER BY name |
There was a problem hiding this comment.
In addition to tests showing that they can be set/removed, how about also adding tests that run a function that is affected by them (e.g. select a date and then show that changing datafusion.format.date changes how they are displayed)?
There was a problem hiding this comment.
@alamb Thanks for the review and yes, we also need to add more functional coverage as follows:
Expected Behavior:
statement ok
SET datafusion.format.date_format = '%d-%m-%Y'
query D
SELECT DATE '2024-03-15'
----
15-03-2024
statement ok
RESET datafusion.format.date_format
query D
SELECT DATE '2024-03-15'
----
2024-03-15
However, currently, sqllogictest framework does not support custom format updates and it runs the test with default format settings and the results as follows:
Current Behavior:
statement ok
SET datafusion.format.date_format = '%d-%m-%Y'
query D
SELECT DATE '2024-03-15'
----
Returns 2024-03-15 instead of 15-03-2024
The reason is that and sqllogictest framework needs to be extended in order to uptake format settings by SessionContext instead default format settings: https://github.com/apache/datafusion/blob/main/datafusion/sqllogictest/src/engines/datafusion_engine/normalize.rs#L239. I need to have a look on this. Does it make sense to be merged current PR and extend sqllogictest framework and adding remaining functional tests by follow-up PR? If so, i can create follow-up issue.
There was a problem hiding this comment.
I need to have a look on this. Does it make sense to be merged current PR and extend sqllogictest framework and adding remaining functional tests by follow-up PR? If so, i can create follow-up issue.
Yes that is an excellent idea. I will do so. Thank you
erenavsarogullari
changed the title
datafusion.format.* configs test coverage
## Which issue does this PR close? - Closes apache#21354. ## Rationale for this change Currently, DataFusion supports 9 `datafusion.format.*` configs but their test coverage seem to be missed so this issue aims to add comprehensive test coverage for them. This is follow-up to recent `config framework` improvements: apache#20372 and apache#20816. ## What changes are included in this PR? New test coverage is being added for `datafusion.format.*` configs. ## Are these changes tested? Yes, new test coverage is being added for `datafusion.format.*` configs. ## Are there any user-facing changes? No
2 participants
Which issue does this PR close?
datafusion.format.*configs test coverage #21354.Rationale for this change
Currently, DataFusion supports 9
datafusion.format.*configs but their test coverage seem to be missed so this issue aims to add comprehensive test coverage for them. This is follow-up to recentconfig frameworkimprovements: #20372 and #20816.What changes are included in this PR?
New test coverage is being added for
datafusion.format.*configs.Are these changes tested?
Yes, new test coverage is being added for
datafusion.format.*configs.Are there any user-facing changes?
No